Skip to content

59462 OIDC par failure #61947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bigred8982
Copy link

Add OIDC event to allow caller to react to OIDC PAR failure during challenge phase

Description

Add new OIDC OnPushAuthorizationFailed, new context for this event, logic to fire event, tests.

Problem

When a validation failure occurs during a PAR request (ex. the request includes an invalid client_id), an OpenIdConnectProtocolException is thrown. This exception bubbles up as an unhandled middleware exception.

Please see issue #59462 for slightly more details.

Goal

We need to give the application the ability to handle the response when a PAR request fails during the challenge phase. Since an exception during the challenge phase bubbles up as a middleware exception, it is difficult for the application to respond. By including a specific OIDC event, the application has the opportunity to redirect the browser to a user-friendly error page.

Example of a web app utilizing this new feature

builder.Services.AddAuthentication(...)
  .AddOpenIdConnect("oidc", options => 
  {
    ...
    options.Events.OnPushAuthorizationFailed = (ctx) => {
        var logger = ctx.HttpContext.RequestServices.GetRequiredService<ILogger<Program>>();
        logger.LogError(ctx.Exception, "Received error while sending PAR request.");
        
        ctx.Response.Redirect("FriendlyErrorPage");
        ctx.Handled = true;
        return Task.CompletedTask;
    };
  });

Fixes #59462

@bigred8982 bigred8982 requested a review from halter73 as a code owner May 14, 2025 22:32
@github-actions github-actions bot added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label May 14, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 14, 2025
@bigred8982
Copy link
Author

@dotnet-policy-service agree company="Tyler Technologies"

@bigred8982
Copy link
Author

Would love feedback from @josephdecock

Also, I'm not sure if this should go into main, but I'd like to see it show up in at least the 10.0 release (it's probably too late to get it into 9).

@bigred8982 bigred8982 marked this pull request as draft May 14, 2025 22:55
@bigred8982 bigred8982 marked this pull request as ready for review May 15, 2025 15:22
@bigred8982
Copy link
Author

If there anything else I need to do process-wise to make this PR ready for review, please let me know. This is my first contribution and I might have accidentally missed a step.

Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 4, 2025
@bigred8982 bigred8982 changed the title Bigred8982/issue 59462 OIDC par failure 59462 OIDC par failure Jun 12, 2025
@bigred8982
Copy link
Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 61947 in repo dotnet/aspnetcore

@Copilot Copilot AI review requested due to automatic review settings August 13, 2025 17:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new OIDC event OnPushAuthorizationFailed to handle PAR (Pushed Authorization Request) failures during the challenge phase. When a PAR request fails with an exception, applications can now handle the response gracefully instead of receiving an unhandled middleware exception.

Key changes:

  • New PushedAuthorizationFailedContext class to provide context for PAR failures
  • New OnPushAuthorizationFailed event in OpenIdConnectEvents
  • Exception handling logic in OpenIdConnectHandler to catch PAR failures and fire the event
  • Test coverage for the new failure handling functionality

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
OpenIdConnectChallengeTests.cs Adds test to verify PAR failure event handling works correctly
PublicAPI.Unshipped.txt Exposes new public API members for the failure context and event
OpenIdConnectHandler.cs Wraps PAR logic in try-catch to fire failure event when exceptions occur
LoggingExtensions.cs Adds logging for handled PAR failure responses
PushedAuthorizationFailedContext.cs New context class providing exception details and handled flag
OpenIdConnectEvents.cs Adds the new failure event to the events collection

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@@ -74,7 +74,7 @@ internal static partial class LoggingExtensions
[LoggerMessage(37, LogLevel.Debug, "The UserInformationReceived event returned Skipped.", EventName = "UserInformationReceivedSkipped")]
public static partial void UserInformationReceivedSkipped(this ILogger logger);

[LoggerMessage(57, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")]
[LoggerMessage(60, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")]
Copy link
Preview

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event ID was changed from 57 to 60, but this appears to be an existing log message that should maintain its original ID. This change could break logging consistency and any code that depends on the specific event ID.

Suggested change
[LoggerMessage(60, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")]
[LoggerMessage(57, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")]

Copilot uses AI. Check for mistakes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were two values set for 57, which I assumed to be a mistake from a previous commit. I updated this to be the next unique number at the time. I am not sure if that was the correct move.

break;
}
}
catch(Exception exception)
Copy link
Preview

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after catch keyword. Should be catch (Exception exception) according to C# formatting conventions.

Copilot uses AI. Check for mistakes.

…horizationFailedContext.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC - Validation failure during PAR does not trigger OnAuthenticationFailed() event
1 participant